Add slog hook #1407
Add slog hook #1407
Conversation
|
I see that CI is failing, and probably needs to be updated. For the tests in this PR to be meaningful, we'll alsoneed to run tests against Go 1.21 (since the new code is behind a build tag). If you'd like me to create a PR to address any of these, please let me know. I'm quite happy to contribute, but don't want to waste my effort if the package maintainers aren't interested 😉 |
|
(did a quick rebase to make CI run) |
mdelapenya
left a comment
mdelapenya
left a comment
There was a problem hiding this comment.
@thaJeztah this LGTM, although I added some minor concerns regarding the usage of a logger Vs a handler. It's basically about not swallowing errors.
@flimzy 👋 Happy to support you here and make progress to eventually merge this into the project.
That was just copied verbatim from the |
|
I've added a test to validate that we report the proper caller, and updated the skip argument accordingly. |
hooks/slog/slog.go
Outdated
| attrs := make([]any, 0, len(entry.Data)) | ||
| for k, v := range entry.Data { | ||
| attrs = append(attrs, slog.Any(k, v)) | ||
| } | ||
| var pcs [1]uintptr | ||
| // skip 8 callers to get to the original logrus caller | ||
| runtime.Callers(8, pcs[:]) |
There was a problem hiding this comment.
Looking if there's some optimisations to be made here; there's also entry.Caller - wondering if that should be used?
There was a problem hiding this comment.
Heh - I was lazy; this is what ChatGPT suggested for this; not entirely sure if the runtime.Callers suggestion is good;
var pc uintptr
if entry.Caller != nil {
pc = entry.Caller.PC
} else {
var pcs [1]uintptr
// Skip runtime.Callers + this function; adjust as needed or make configurable.
runtime.Callers(2, pcs[:])
pc = pcs[0]
}
r := slog.NewRecord(entry.Time, lvl, entry.Message, pc)
if len(entry.Data) != 0 {
attrs := make([]slog.Attr, 0, len(entry.Data))
for k, v := range entry.Data {
attrs = append(attrs, slog.Any(k, v))
}
r.AddAttrs(attrs...)
}
return handler.Handle(ctx, r)There was a problem hiding this comment.
We definitely can do that, but it subtly changes behavior. Perhaps in a good way?
If we use entry.Caller.PC, then to get source= in the final output, both Logrus and the slog handler need to be configured to show sources. With the current implementation, the behavior depends only on slog's configuration.
I'm not sure which is "more correct". I suppose in practice, they'll likely both be enabled or both disabled. So I'll make the change to use entry.Caller.PC unless you feel that's not appropriate :)
3 participants
This addresses half of #1401. The easy half, honestly.
I have an implementation of the other half we've been using in our own project, but it will need some polish before making it public, so I'm submitting this PR now to gauge interest of the project maintainers. If we like this, I'll work on polishing the other half in a later PR.